Conversation
GitHub Actions
Error
github-actions
Bot
added
kind/protocol
|
This pull request may overlap with #1977. Both touch EGFX ResetGraphics handling of the bitmap cache: PR 1977 describes correcting which caches a ResetGraphics clears to fix stale pixels, while this change keeps the cache across reset under the MS-RDPEGFX 3.3.1.4 size cap in the same compositor. Shared scope in one area for human assessment. This notice is advisory only. Automated review continues as usual, and how these pull requests relate is for maintainers and authors to decide. Note LLM-assisted content (no human feedback). |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR keeps the egfx bitmap cache across ResetGraphics and recomputes allocated_bytes to the surviving cache charge. Independently verified: the recompute is exact (copy_region always yields w*h*4 per charged tile), every other charged pool (surfaces, frame, ready) is cleared with its charge, and cache retention matches MS-RDPEGFX 3.3.5.14, consistent with existing Progressive/ClearCodec handling; tests cover accounting and a post-reset CacheToSurface. No protocol or correctness defects. The single valid candidate (skeptical, reset-retains-cache-budget-charge) is refined: post-reset surface creation competes with retained tiles under the shared 256 MiB budget and refusal is a silent no-op; the refinement corrects the claim that the trade-off is unacknowledged (the PR body mentions the retained budget share) and the recovery wording. Protocol and code-compressor reported no findings.
|
On the overlap notice: #1977 contains the same |
There was a problem hiding this comment.
The PR keeps the EGFX bitmap cache (and its byte charge) across ResetGraphics in Compositor::reset, matching MS-RDPEGFX 3.3.5.14 (reset only resizes the output buffer) and the crate's existing treatment of Progressive/ClearCodec contexts across resets. The accounting recomputation is correct: the surface, frame and ready pools are cleared along with their vectors, so only the cache charge remains, and the test verifies the kept tile still produces output via CacheToSurface. Both specialist findings are valid and published: a doc-only mis-citation of the cache messages as 2.2.2.10/2.2.2.11 (low), and a medium resilience regression where a hostile peer can pin the shared 256 MiB budget in u16-keyed cache slots and now stays pinned across resets, permanently starving post-reset surface creation and output materialization. code-compressor reported no findings.
github-actions
Bot
added
ai-reviewed/2
…#2007) Windows lists every dynamic channel it intends to move in its Soft-Sync request, including the ones the client declined with NO_LISTENER. Against a Windows 11 host the request lists channels 2, 6, 7, 8, 9, 10, 11 and 12 (CoreInput, MouseCursor, Graphics, Video, Geometry, ...), and only channel 7, the graphics pipeline, is open. `process_soft_sync_request` dropped a whole channel list as soon as one ID in it was not open. The tunnel was then never switched, and the channels the client had opened stayed on TCP while the server was already sending them on the tunnel (MS-RDPEDYC 3.2.5.3.1). Unopened channels are now skipped one by one, and the tunnel is switched for the rest. ## Testing - New `dvc::client::soft_sync_skips_channels_the_client_did_not_open` in `ironrdp-testsuite-core`. - Live, against a Windows 11 host over RDP-UDP version 2, with the viewer built from a branch that also carries the tunnel and client PRs of this series: the Soft-Sync request above now switches the tunnel, and the graphics pipeline moves onto it. ## Checks - `cargo fmt --all -- --check` - `cargo clippy --workspace --all-targets --features helper,__bench --locked -- -D warnings` - `cargo test --locked -p ironrdp-testsuite-core -p ironrdp-testsuite-extra`, plus the lib tests of the crates touched here - `cargo test --workspace --locked` on a branch that merges this PR with the other Windows interop PRs from this series - `typos` on the changed files ## Series These PRs port the Windows interop fixes and Linux backends from a downstream IronRDP fork, so the fork can be retired. Each one is based on `master` and can be reviewed and merged on its own. I also checked that all of them merge cleanly together in this order. - #2007 fix(dvc): Soft-Sync tunnel with declined channels - #2008 fix(session)!: channels and graphics on the tunnel - #2009 fix(rdpeudp): auto-detect on the tunnel - #2010 fix(graphics)!: SRL streams from Windows - #2011 fix(egfx): bitmap cache across ResetGraphics - #2012 feat(session): bandwidth measurements during the session - #2013 feat(client): graphics pipeline and RDP-UDP version options - #2014 fix(client): resize reconnects on the graphics pipeline - #2015 feat(client): transport event - #2016 feat(cliprdr): Linux clipboard backend - #2017 feat(rdpdr): printer on Linux and macOS Co-authored-by: AKolenda <testedemail2222@gmail.com>
The compositor emptied the bitmap cache on every ResetGraphics. Windows sends a ResetGraphics for every desktop resize and afterwards keeps pasting toolbars, icons and text with CacheToSurface from slots it filled before the reset, so those regions stayed black after each resize until something forced a fresh upload. The bitmap cache is not part of the graphics output: MS-RDPEGFX 3.3.5.14 only resizes the Graphics Output Buffer, and slots are released by EvictCacheEntry, a cache import or the end of the channel. This is the same reasoning the client already applies to the Progressive and ClearCodec contexts. The reset now keeps the cache and its share of the allocation budget, and still drops surfaces and pending output.
MS-RDPEGFX 3.3.1.4 caps the bitmap cache at 100 MB, or 16 MB with SMALL_CACHE, so the charge kept across ResetGraphics leaves a conforming server room in the budget for the surfaces it creates after the reset.
MS-RDPEGFX 3.3.1.4 caps the bitmap cache at 100 MB. A server that filled the cache past that cap kept it across ResetGraphics, together with its share of the compositor budget, and could leave no room for the surfaces it creates after the reset. Such a cache is now dropped with the surfaces, and a cache within the cap is kept as before. Also cite the bitmap cache as MS-RDPEGFX 3.3.1.4 and SurfaceToCache as 2.2.2.6. The comments cited 2.2.2.10 and 2.2.2.11, which are DeleteSurface and StartFrame.
fca225a to
46ee89b
Compare
github-actions
Bot
added
needs-review
|
Rechecked all three review findings against 46ee89b. The cache citations are corrected, caches over 100 MiB are dropped on reset, and retained cache charges leave at least 156 MiB of the shared 256 MiB budget for surfaces. The regression reset_drops_a_cache_over_the_protocol_cap verifies that an overfilled cache cannot starve post-reset surfaces. Reran all 29 compositor unit tests; all passed. Current CI also passes. No further code changes were needed, and the three outdated review threads are now addressed. |
The compositor emptied the bitmap cache on every ResetGraphics. Windows reuses cached toolbars, icons and text after a resize, so subsequent CacheToSurface commands could leave those regions black until a fresh upload.
Compositor::resetnow drops surfaces and pending output while retaining the bitmap cache and its allocation charge. MS-RDPEGFX 3.3.5.14 resets the graphics output; the bitmap-cache state is described separately in 3.3.1.4. To preserve space for replacement surfaces in the 256 MiB budget, a cache exceeding the protocol's 100 MiB maximum is discarded during reset.The regression tests verify that cached content remains renderable after reset, surface allocation charges are released, retained cache charges remain accounted for, and an oversized cache is discarded.
Validation
Rechecked this revision: all 29 compositor unit tests pass, and the current regular CI suite is green. The review's specification-reference corrections and allocation-cap safeguard are already included in 46ee89b.